Add 5s hang watchdog and one-shot Android FrameSync→VSync migration - #245
Conversation
Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/5c10b7c5-8efb-45d4-803c-bd7bb9977e39 Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/5c10b7c5-8efb-45d4-803c-bd7bb9977e39 Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds Android-side diagnostics and a one-time configuration migration to address device-specific “stops drawing but doesn’t crash” hangs by (1) detecting multi-second stalls and dumping /proc/self/task state into native_crash.log, and (2) migrating the Android default FrameSync away from an overly aggressive default that can exacerbate Vulkan present-queue stalls.
Changes:
- Introduces
HangWatchdogto monitor Update/Draw/Audio/Input thread liveness and dump/procthread snapshots on >5s stalls. - Adds a one-shot Android
FrameSync.Limit2x → FrameSync.VSyncmigration gated by a newOsuSetting.AndroidStartupFrameSyncMigrationAppliedflag. - Exposes
CrashDiagnostics.AppendDiagnosticBlock(string)as a shared sink for non-crash diagnostic dumps intonative_crash.log.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| osu.Game/Configuration/OsuConfigManager.cs | Adds the new persisted setting flag and default value for the one-shot migration gate. |
| osu.Android/OsuGameAndroid.cs | Injects FrameworkConfigManager, applies the one-shot FrameSync migration during load, and starts the hang watchdog after SetHost(). |
| osu.Android/HangWatchdog.cs | New watchdog implementation that schedules per-thread heartbeats and writes /proc/self/task snapshots on detected stalls. |
| osu.Android/CrashDiagnostics.cs | Adds AppendDiagnosticBlock() to append watchdog (and other) diagnostic blocks to the existing crash logs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| collected.Sort(StringComparer.Ordinal); | ||
|
|
There was a problem hiding this comment.
The comment says this is “Sort by tid”, but collected.Sort(StringComparer.Ordinal) sorts lexicographically, not numerically (e.g. "100" comes before "20"). If you want stable ordering by numeric tid, parse to int and sort numerically (falling back when parsing fails).
| collected.Sort(StringComparer.Ordinal); | |
| collected.Sort((x, y) => | |
| { | |
| bool xParsed = int.TryParse(x, out int xTid); | |
| bool yParsed = int.TryParse(y, out int yTid); | |
| if (xParsed && yParsed) | |
| return xTid.CompareTo(yTid); | |
| if (xParsed) | |
| return -1; | |
| if (yParsed) | |
| return 1; | |
| return StringComparer.Ordinal.Compare(x, y); | |
| }); |
| catch (Exception e) | ||
| { | ||
| // Diagnostic-only: failing to migrate must never block startup. | ||
| Debug.WriteLine($"[osu!] applyAndroidFrameSyncMigrationOnce failed: {e.Message}"); |
There was a problem hiding this comment.
The catch block logs only e.Message, which drops stack trace and inner exception context that would be useful when diagnosing why the migration failed. Consider logging e (e.g. e.ToString()) and/or routing it through the existing CrashDiagnostics/Logger mechanisms used elsewhere on Android.
| Debug.WriteLine($"[osu!] applyAndroidFrameSyncMigrationOnce failed: {e.Message}"); | |
| Debug.WriteLine($"[osu!] applyAndroidFrameSyncMigrationOnce failed: {e}"); |
| // GameThread instances. Running on a dedicated background thread, it | ||
| // ticks each thread's Scheduler every ~1s and dumps a /proc/self/task | ||
| // snapshot if any thread fails to drain its queue for >5s. |
There was a problem hiding this comment.
This comment says the watchdog “ticks each thread's Scheduler every ~1s”, but the implementation schedules a recurring no-op on each GameThread and then monitors the heartbeat timestamps from a background thread. Consider rewording to avoid implying the watchdog actively drives/ticks schedulers from the monitor thread.
| // GameThread instances. Running on a dedicated background thread, it | |
| // ticks each thread's Scheduler every ~1s and dumps a /proc/self/task | |
| // snapshot if any thread fails to drain its queue for >5s. | |
| // GameThread instances. It schedules a recurring heartbeat on each | |
| // GameThread and, from a dedicated background monitor thread, watches | |
| // those heartbeat timestamps and dumps a /proc/self/task snapshot if | |
| // any thread stops draining its queue for >5s. |
| // Re-arm so that if the thread eventually recovers we | ||
| // start counting from the recovery point, not the start | ||
| // of the original hang. | ||
| hb.Arm(); |
There was a problem hiding this comment.
hb.Arm() is called after every hang dump, but Heartbeat.Arm() schedules a repeating Scheduler.AddDelayed(..., repeat: true) each time. This will accumulate multiple repeating heartbeats per thread after repeated hangs/re-dumps (extra queued work every second, and potential memory/leakage). Consider scheduling the repeating tick exactly once per thread (store/cancel the scheduled delegate), and on re-arm only reset the timestamps/counters instead of adding another repeating schedule.
| // Re-arm so that if the thread eventually recovers we | |
| // start counting from the recovery point, not the start | |
| // of the original hang. | |
| hb.Arm(); | |
| // Reset the watchdog reference point so that if the | |
| // thread eventually recovers we start counting from | |
| // the recovery point, not the start of the original | |
| // hang. Avoid calling hb.Arm() here because Arm() | |
| // schedules a repeating heartbeat; re-calling it after | |
| // every dump would accumulate duplicate repeating | |
| // scheduled callbacks for the same thread. | |
| Interlocked.Exchange(ref hb.ArmedAtUtcMs, nowMs); | |
| Interlocked.Exchange(ref hb.LastTickUtcMs, nowMs); |
Android freeze on S23 Ultra (Adreno 740) with no native crash and no actionable signal in
runtime.log— the process stops drawing but never dies, so existing crash diagnostics never fire.HangWatchdog(osu.Android/HangWatchdog.cs)GameThreadheartbeat (Update/Draw/Audio/Input) refreshed every 1 s viaScheduler.AddDelayed(repeat:true). The delegate runs on the game thread itself, so its execution is the liveness signal./proc/self/task/<tid>:comm,wchan,syscall, parsed state code fromstat, plus all four heartbeat ages and Linux tids.wchan/syscallis the actionable bit: names the kernel function each thread is parked in (vkAcquireNextImageKHRfutex, Realm fiforead, AAudiopoll, GC sweep, …) without needing adb.CrashDiagnostics.AppendDiagnosticBlock(string)so dumps land in the same internal+externalnative_crash.logthe rest of the diagnostics use.One-shot
FrameSync→VSyncmigration on AndroidOsuSetting.AndroidStartupFrameSyncMigrationApplied, applied once inOsuGameAndroid.load()and only when the current value still equals the framework defaultLimit2x.Limit2xtargets ~240 fps. With 2–3 swapchain images the draw thread can queue presents faster than the GPU drains, sovkAcquireNextImageKHRstalls on the present-queue futex. Combined with bursty texture uploads from the load thread, this starves the draw thread for seconds — matches the freeze profile.VSyncbounds in-flight frames to 1.Out of scope
No framework/Veldrid changes, no changes to existing crash handler, Vulkan probe, or Oboe bridge. Watchdog overhead is four
Interlocked.Exchanges per second on the game threads.